Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[r2r] broadcast tx to txhlp / refactor tx errors #1245

Merged
merged 50 commits into from
May 4, 2022

Conversation

onur-ozkan
Copy link
Member

@onur-ozkan onur-ozkan commented Apr 5, 2022

  • Implement P2P transaction broadcast helper. #1238
  • refactor TransactionFut's error type(to recover transactions in some error cases.).
  • write tests for functions that may return TransactionErr::TxRecoverableError
  • Add txhlp/TICKER P2P subscription when UTXO coins are activated in native mode.

Signed-off-by: ozkanonur <work@onurozkan.dev>
Signed-off-by: ozkanonur <work@onurozkan.dev>
@onur-ozkan onur-ozkan requested a review from artemii235 April 5, 2022 12:45
Signed-off-by: ozkanonur <work@onurozkan.dev>
@onur-ozkan onur-ozkan force-pushed the p2p-transaction-helper-broadcast branch from 0a6d0c9 to 43862b6 Compare April 5, 2022 16:08
Signed-off-by: ozkanonur <work@onurozkan.dev>
Signed-off-by: ozkanonur <work@onurozkan.dev>
Signed-off-by: ozkanonur <work@onurozkan.dev>
Signed-off-by: Onur Özkan <work@onurozkan.dev>
Signed-off-by: ozkanonur <work@onurozkan.dev>
Copy link
Member

@artemii235 artemii235 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the prompt implementation! It's the first review iteration 🙂

mm2src/coins/utxo/slp.rs Outdated Show resolved Hide resolved
mm2src/coins/eth.rs Outdated Show resolved Hide resolved
mm2src/lp_network.rs Outdated Show resolved Hide resolved
@@ -1330,6 +1338,13 @@ impl TakerSwap {
},
};

broadcast_transaction_message(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have noticed that it's better to broadcast_transaction_message also if send_taker_refunds_payment and other fns fail. Please make send_taker_refunds_payment and others to also return the generated transaction in case of error and call broadcast_transaction_message for it.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

trait SwapOps functions are (only TransactionFut returners) now calling broadcast_transaction_message right after generating transaction.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a little confused 😅
Could you please tell where trait SwapOps functions call broadcast_transaction_message? I couldn't find it in the changes

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a little confused sweat_smile Could you please tell where trait SwapOps functions call broadcast_transaction_message? I couldn't find it in the changes

Sorry, I couldn't explain correctly. I didn't call broadcast_transaction_messageinside of the trait SwapOps functions, I call it in maker_swap and taker_swap after each trait SwapOps functions. But I guess Artem is pointing different thing than what I did here. I will update the changes today.

Signed-off-by: ozkanonur <work@onurozkan.dev>
Signed-off-by: ozkanonur <work@onurozkan.dev>
Signed-off-by: ozkanonur <work@onurozkan.dev>
Signed-off-by: ozkanonur <work@onurozkan.dev>
Signed-off-by: ozkanonur <work@onurozkan.dev>
@onur-ozkan onur-ozkan requested a review from artemii235 April 6, 2022 11:51
…slp_htlc_should_fail`

Signed-off-by: ozkanonur <work@onurozkan.dev>
@@ -2022,7 +2016,7 @@ mod slp_tests {

let err = fusd.send_raw_tx(&tx_bytes_str).wait().unwrap_err();
println!("{:?}", err);
assert!(err.contains("is not valid with reason outputs greater than inputs"));
assert!(err.contains("the transaction was rejected by network rules.\\n\\ntransaction already in block chain"));
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This had to change since send_raw_tx now calling uxto_common::send_raw_tx here @artemii235

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ozkanonur Very good that we have this test 🙂
I forgot that SLP requires additional transaction validation and provided incorrect comment that utxo_common::send_raw_tx_bytes can be used for it. Due to the missing validation, the invalid transaction was broadcast - with SLP protocol, it effectively means tokens burning (luckily, test net tokens in this case). More info on the topic: https://slp.dev/guides/slp-implementation-instructions/#have-multiple-sources-of-validation-or-client-side-validation

The usage of validation code
https://github.com/KomodoPlatform/atomicDEX-API/blob/ff6fc9f6722cf4f8ac0a290043deded201e7dc59/mm2src/coins/utxo/slp.rs#L1037-L1040

Could you please roll back the send_raw_tx implementation and use broadcast_tx in the Slp::send_raw_tx_bytes too? Please also extend this test and check that send_raw_tx_bytes will return is not valid with reason outputs greater than inputs for this tx.

Also, if the test started to fail, it's undesirable to change it - there's a very high chance that tested code got broken.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, if the test started to fail, it's undesirable to change it - there's a very high chance that tested code got broken.

PS, even if I asked to change the code on the review 🙂 I can be wrong.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

Signed-off-by: ozkanonur <work@onurozkan.dev>
Signed-off-by: ozkanonur <work@onurozkan.dev>
Signed-off-by: ozkanonur <work@onurozkan.dev>
Copy link

@sergeyboyko0791 sergeyboyko0791 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great progress! Please take a look at my suggestions

mm2src/coins/solana/solana_tests.rs Outdated Show resolved Hide resolved
mm2src/coins/solana/spl_tests.rs Outdated Show resolved Hide resolved
mm2src/coins/utxo/slp.rs Outdated Show resolved Hide resolved
mm2src/coins/utxo/slp.rs Show resolved Hide resolved
mm2src/coins/utxo/slp.rs Outdated Show resolved Hide resolved
mm2src/coins/utxo/slp.rs Show resolved Hide resolved
mm2src/coins/utxo/slp.rs Outdated Show resolved Hide resolved
mm2src/lp_network.rs Outdated Show resolved Hide resolved
mm2src/lp_swap/maker_swap.rs Outdated Show resolved Hide resolved
mm2src/lp_swap/taker_swap.rs Outdated Show resolved Hide resolved
Signed-off-by: ozkanonur <work@onurozkan.dev>
Signed-off-by: Onur Özkan <work@onurozkan.dev>
Copy link
Collaborator

@shamardy shamardy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we need to send the P2P message when broadcasting lightning-related transactions too. The message broadcasting should be added to this function https://github.com/KomodoPlatform/atomicDEX-API/blob/5c24769605a3ce7ea6c336ffff7aeec351e7705c/mm2src/coins/lightning/ln_rpc.rs#L63

@onur-ozkan
Copy link
Member Author

I think we need to send the P2P message when broadcasting lightning-related transactions too. The message broadcasting should be added to this function

https://github.com/KomodoPlatform/atomicDEX-API/blob/5c24769605a3ce7ea6c336ffff7aeec351e7705c/mm2src/coins/lightning/ln_rpc.rs#L63

Need MmArc in order to send p2p message.

@shamardy
Copy link
Collaborator

Need MmArc in order to send p2p message.

I see, then I guess I better handle this case once this PR is merged since I changed the implementation of the BroadcasterInterface a bit in the lightning history PR. Maybe I can add P2PContext to the Platform struct that implements BroadcasterInterface as a solution, will look for the best solution then.

@onur-ozkan onur-ozkan changed the title [r2r] broadcast tx to txhlp / refactor tx errors [wip] broadcast tx to txhlp / refactor tx errors Apr 20, 2022
Signed-off-by: ozkanonur <work@onurozkan.dev>
Signed-off-by: ozkanonur <work@onurozkan.dev>
@onur-ozkan onur-ozkan changed the title [wip] broadcast tx to txhlp / refactor tx errors [r2r] broadcast tx to txhlp / refactor tx errors Apr 20, 2022
Signed-off-by: ozkanonur <work@onurozkan.dev>
Copy link
Member

@artemii235 artemii235 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One change.

mm2src/rpc/lp_commands.rs Outdated Show resolved Hide resolved
Signed-off-by: Onur Özkan <work@onurozkan.dev>
Copy link
Member

@artemii235 artemii235 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've noticed that few changes do not meet the initial requirement 🙂

mm2src/lp_swap/taker_swap.rs Outdated Show resolved Hide resolved
mm2src/lp_swap/taker_swap.rs Outdated Show resolved Hide resolved
mm2src/lp_swap/taker_swap.rs Outdated Show resolved Hide resolved
mm2src/lp_swap/taker_swap.rs Show resolved Hide resolved
mm2src/lp_swap/maker_swap.rs Outdated Show resolved Hide resolved
mm2src/lp_swap/maker_swap.rs Show resolved Hide resolved
Signed-off-by: ozkanonur <work@onurozkan.dev>
Copy link
Member

@artemii235 artemii235 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One minor change

mm2src/coins/solana.rs Show resolved Hide resolved
artemii235
artemii235 previously approved these changes Apr 22, 2022
Copy link
Member

@artemii235 artemii235 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔥

@artemii235
Copy link
Member

artemii235 commented Apr 25, 2022

Waiting for @sergeyboyko0791 approval.

Copy link

@sergeyboyko0791 sergeyboyko0791 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the late review.
It may take some time to consider my suggestions, please ping me when it's ready for the next review iteration.


Box::new(
self.send_to_address(address, try_fus!(wei_from_big_decimal(&amount, self.decimals)))
self.send_to_address(address, try_tx_fus!(wei_from_big_decimal(&amount, self.decimals)))
.map_err(TransactionErr::PlainError)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

EthCoin::send_to_address should return TransactionErr::TxRecoverableError. Please look at the sign_and_send_transaction_impl function:
https://github.com/KomodoPlatform/atomicDEX-API/blob/c23b1f7bb2941b51297709aee3ac3c46f5370c6f/mm2src/coins/eth.rs#L1382

mm2src/coins/eth.rs Show resolved Hide resolved
mm2src/coins/eth.rs Show resolved Hide resolved
Comment on lines 759 to 760
let signed = try_tx_fus!(SignedEthTx::new(tx));
let swap_contract_address = try_tx_fus!(swap_contract_address.try_to_address());

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you think, should we return TransactionErr::TxRecoverableError if swap_contract_address.try_to_address() fails?
Please also check if we should do that in other places.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should, why not if we can just return it right?

mm2src/coins/eth.rs Show resolved Hide resolved
}
}

if now_ms() / 1000 > wait_until {
return ERR!(
return TX_ERR!(

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems that TX_ERR returns TransactionErr::PlainError. So what do you think about renaming it to TX_PLAIN_ERR?
Also I'd suggest using snake_case for macros. I know that we use ERR a lot, so that there is no confusion, we can leave it as SCREAMING_SNAKE_CASE. I leave it up to you :)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, macros normally have snake_case naming convention, but here we are providing a compatibility for a macro that is already exists. We have to change all of them to avoid confusion(which we will do for #1247 ).

}
}

pub type TransactionFut = Box<dyn Future<Item = TransactionEnum, Error = TransactionErr> + Send>;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you please consider this comment? #1258 (comment)
If you have a different point of view, I'll accept it 🙂

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think both ways are fine. Declaring on top of the module or just keep them close to it's couples. But, have to apply only one of them in the project, otherwise it will complicate things more.

@artemii235 which way we should follow? The choice will effect all the project (either all the types will move to the top, or will move to it's couple structs/functions).

@@ -1587,6 +1656,21 @@ impl Deref for MmCoinEnum {
}
}

impl MmCoinEnum {
pub fn is_utxo_and_in_native_mode(&self) -> bool {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should it be is_utxo_in_native_mode instead?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should it be is_utxo_in_native_mode instead?

Yeah, damn what was I thinking 🤣

Comment on lines 465 to 467
return Err(TransactionErr::TxRecoverableError(
TransactionEnum::from(signed),
ERRL!("{:?}", err),

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about using a macro like:

/// `TransactionErr:TxRecoverableError` compatible `ERR` macro.
macro_rules! TX_RECOVERABLE_ERR {
    ($tx: expr, $format: expr, $($args: tt)+) => {
        Err(crate::TransactionErr::TxRecoverableError(TransactionEnum::from($tx), ERRL!($format, $($args)+)))
    };
    ($tx: expr, $format: expr) => {
        Err(crate::TransactionErr::TxRecoverableError(TransactionEnum::from($tx), ERRL!($format)))
    }
}

This macro can be used elsewhere

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah that's actually very good idea

@@ -1083,10 +1100,15 @@ impl MarketCoinOps for Qrc20Coin {
from_block: u64,
_swap_contract_address: &Option<BytesJson>,
) -> TransactionFut {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that the MarketCoinOps::wait_for_tx_spend method shouldn't return TransactionErr because it actually doesn't broadcast any transaction to the blockchain network.
I'd add another Transaction future that returns String error instead of TransactionErr.
It's only my opinion, so it's up to you to change or not

Copy link
Member Author

@onur-ozkan onur-ozkan Apr 29, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can rename TransactionErr into TransactionFutErr to make it more general. In this case, you don't need to create another error type since we can use TransactionFutErr::Plain. Is this okay for you?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think TransactionErr fits better, because we have functions that don't return futures.

https://github.com/KomodoPlatform/atomicDEX-API/blob/9444edabe014b1d5519e1784159f9234e87f16ce/mm2src/coins/utxo/slp.rs#L412

I'd rather have TransactionFut = Box<dyn Future<Item = TransactionEnum, Error = String>> and TxRecoverableFut = Box<dyn Future<Item = TransactionEnum, Error = TransactionFutErr>>.
What do you think?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But on the other hand, it may lead to confusion in macros, because try_tx_fus will actually return TxRecoverableFut. So, as I said, it's up to you :)

@onur-ozkan onur-ozkan changed the title [r2r] broadcast tx to txhlp / refactor tx errors [wip] broadcast tx to txhlp / refactor tx errors Apr 27, 2022
Signed-off-by: ozkanonur <work@onurozkan.dev>
@onur-ozkan onur-ozkan changed the title [wip] broadcast tx to txhlp / refactor tx errors [r2r] broadcast tx to txhlp / refactor tx errors Apr 29, 2022
@onur-ozkan
Copy link
Member Author

@sergeyboyko0791 ready another iteration :)

Copy link

@sergeyboyko0791 sergeyboyko0791 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the fixes! Second review iteration

mm2src/coins/qrc20.rs Outdated Show resolved Hide resolved
mm2src/coins/qrc20/qrc20_tests.rs Outdated Show resolved Hide resolved
mm2src/coins/qrc20/qrc20_tests.rs Show resolved Hide resolved
mm2src/coins/solana.rs Outdated Show resolved Hide resolved
mm2src/coins/utxo.rs Outdated Show resolved Hide resolved
mm2src/coins/utxo/utxo_common.rs Outdated Show resolved Hide resolved
@@ -887,7 +893,7 @@ impl SwapOps for ZCoin {
script_data,
key_pair.private().secret.as_slice(),
);
let tx = try_s!(tx_fut.await);
let tx = try_tx_s!(tx_fut.await);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't z_p2sh_spend return TransactionFutErr?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I didn't understand what exactly you mean here, it's already returning TransactionFutErr.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I meant that z_p2sh_spend currently returns ZP2SHSpendError, but it could return an error extended by the TxRecoverable variant or something like that. It actually can return here
https://github.com/KomodoPlatform/atomicDEX-API/blob/5ba0dd1684ce879b4724c6c9badcbe524537f228/mm2src/coins/z_coin/z_htlc.rs#L164-L167
So maybe we can add ZP2SHSpendError::TxRecoverable 🤔

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I meant that z_p2sh_spend currently returns ZP2SHSpendError, but it could return an error extended by the TxRecoverable variant or something like that. It actually can return here

https://github.com/KomodoPlatform/atomicDEX-API/blob/5ba0dd1684ce879b4724c6c9badcbe524537f228/mm2src/coins/z_coin/z_htlc.rs#L164-L167

So maybe we can add ZP2SHSpendError::TxRecoverable

Implemented it now.

@artemii235 I can rollback the commit if this isn't necessary.

mm2src/docker_tests/qrc20_tests.rs Outdated Show resolved Hide resolved
mm2src/lp_swap/maker_swap.rs Show resolved Hide resolved
@@ -1142,6 +1187,7 @@ impl MakerSwap {
Ok(Some(FoundSwapTxSpend::Spent(_))) => {
log!("Warning: MakerPayment spent, but TakerPayment is not yet. Trying to spend TakerPayment");
let transaction = try_s!(try_spend_taker_payment(self, &secret_hash.0).await);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One question to the MakerSwap::recover_funds method - should we broadcast tx message in this case?

Copy link
Member Author

@onur-ozkan onur-ozkan Apr 30, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can't since try_spend_taker_payment doesn't' return recoverable tx.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I couldn't comment on this snippet:
https://github.com/KomodoPlatform/atomicDEX-API/blob/f87cef3ba6d7802fcb720e4a69e43c27f653e14b/mm2src/lp_swap/maker_swap.rs#L1110-L1122
So I left the link
What do you think about send_maker_spends_taker_payment?

Signed-off-by: Onur Özkan <work@onurozkan.dev>
Signed-off-by: ozkanonur <work@onurozkan.dev>
Copy link

@sergeyboyko0791 sergeyboyko0791 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few minor changes please 🙂
Also I've noticed that swap methods can be refactored with MmError, so we could avoid adding some macros. But it's overhead for this PR, so let's plan to refactor them someday later.

@@ -149,19 +145,44 @@ macro_rules! try_tx_s {
};
}

/// `TransactionFutErr:Plain` compatible `ERR` macro.
/// `ZP2SHSpendError` compatible `TransactionErr` handling macro.
macro_rules! try_ztx_s {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's better to move the macro to z_coin.rs since it's compatible to ZCoin only. What do you think?

Comment on lines 161 to 165
return Err(crate::TransactionErr::Plain(format!(
"{}:{}] {:?}",
file!(),
line!(),
err

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It can be simplified as:

return Err(crate::TransactionErr::Plain(ERRL!("{:?}", err)))

@@ -458,11 +458,8 @@ impl Qrc20Coin {
.generate_qrc20_transaction(outputs)
.await
.mm_err(|e| e.into_withdraw_error(platform, decimals))

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you please replace mm_err with map_err here? The error trace is collected on the line below

.map_err(|e| TransactionErr::Plain(ERRL!("{}", e)))?;

So there is no need to convert the error to MmError.

Signed-off-by: Onur Özkan <work@onurozkan.dev>
@onur-ozkan
Copy link
Member Author

A few minor changes please slightly_smiling_face Also I've noticed that swap methods can be refactored with MmError, so we could avoid adding some macros. But it's overhead for this PR, so let's plan to refactor them someday later.

I think this PR is already on the overhead stage so yeah it would be better to seperate refactoring tasks to another PR. 🙂

Fixed the notes.

Copy link

@sergeyboyko0791 sergeyboyko0791 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great! LGTM 🔥

@artemii235 artemii235 merged commit 9e622d5 into dev May 4, 2022
@artemii235 artemii235 deleted the p2p-transaction-helper-broadcast branch May 4, 2022 11:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants